Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: enhanced in-place update module to support vertical scaling #1353

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: enhanced in-place update module to support vertical scaling #1353

wants to merge 5 commits into from
+3,360 −66

Conversation

LavenderQAQ
Copy link

@LavenderQAQ LavenderQAQ commented Aug 2, 2023 •
edited
Loading

Ⅰ. Describe what this PR does

Enhanced the in-place update module to enable kruise's workload to modify resources without restarting the pod.

Ⅱ. Does this pull request fix one issue?

Fixes #1212

Ⅲ. Describe how to verify it

Need to kubernetes v1.27 version, and open feature gate InPlacePodVerticalScaling. Here is my kind configuration:

kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
featureGates:
  "InPlacePodVerticalScaling": true
nodes:
- role: control-plane
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72
- role: worker
  image: kindest/node:v1.27.3@sha256:3966ac761ae0136263ffdb6cfd4db23ef8a83cba8a463690e98317add2c9ba72

Build a cloneset (or other workload that includes in-place updates) and set updateStrategy to InPlaceIfPossible. Here's an example:

apiVersion: apps.kruise.io/v1alpha1
kind: CloneSet
metadata:
  name: nginx-cloneset
spec:
  updateStrategy:
    type: InPlaceIfPossible
  replicas: 3
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx-1
        image: nginx:1.21.1
        ports:
        - containerPort: 80
        resources:
          limits:
            cpu: "1"
            memory: "256Mi"
          requests:
            cpu: "0.5"
            memory: "128Mi"

Change the workload's resources, wait a while, and you'll find that the pod's resources will change, and the pod won't restart.

Ⅳ. Special notes for reviews

The control of Status for vertical scaling requires a higher version of the k8s api, which makes this pr need to wait for kruise version upgrade. It cannot be merged at this time because it is not yet complete.

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign veophi for approval by writing /assign @veophi in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sonatype-lift
Copy link

sonatype-lift bot commented Aug 2, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Aug 2, 2023
@LavenderQAQ LavenderQAQ changed the title feat: enhanced in-place update module to support vertical scaling feat: enhanced inplace update module to support vertical scaling Aug 2, 2023
@LavenderQAQ LavenderQAQ changed the title feat: enhanced inplace update module to support vertical scaling feat: enhanced in-place update module to support vertical scaling Aug 2, 2023
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 4 times, most recently from 08c47c7 to 04b5eb8 Compare August 7, 2023 03:04
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023 •
edited by codecov bot
Loading

Codecov Report

Attention: Patch coverage is 72.50000% with 55 lines in your changes missing coverage. Please review.

Project coverage is 49.40%. Comparing base (0d0031a) to head (eea019c).
Report is 85 commits behind head on master.

Files with missing lines Patch % Lines
pkg/util/inplaceupdate/inplace_update_defaults.go 77.00% 17 Missing and 6 partials ⚠️
pkg/util/inplaceupdate/inplace_update_vertical.go 76.74% 11 Missing and 9 partials ⚠️
pkg/controller/cloneset/core/cloneset_core.go 0.00% 6 Missing ⚠️
...kg/controller/cloneset/sync/cloneset_sync_utils.go 0.00% 3 Missing ⚠️
pkg/util/inplaceupdate/inplace_update.go 40.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
+ Coverage   47.91%   49.40%   +1.49%     
==========================================
  Files         162      192      +30     
  Lines       23491    19752    -3739     
==========================================
- Hits        11256     9759    -1497     
+ Misses      11014     8730    -2284     
- Partials     1221     1263      +42
Flag Coverage Δ
unittests 49.40% <72.50%> (+1.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from 0470ce2 to 4e13c0d Compare August 7, 2023 03:56
@kruise-bot kruise-bot added needs-rebase size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Aug 8, 2023
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from 35c4939 to 28406cb Compare August 8, 2023 11:46
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 2 times, most recently from 48126ff to aca435f Compare August 8, 2023 12:04
@LavenderQAQ LavenderQAQ marked this pull request as ready for review August 8, 2023 13:16
@kruise-bot kruise-bot requested a review from veophi August 8, 2023 13:16
@LavenderQAQ
Copy link
Author

LavenderQAQ commented Aug 8, 2023 •
edited
Loading

The complete logic needs to wait until the k8s api of kruise is upgraded.

@LavenderQAQ
Copy link
Author

/hold

@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from aca435f to d064af1 Compare August 8, 2023 13:24
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch from d064af1 to cbd728f Compare August 15, 2023 08:01
@LavenderQAQ LavenderQAQ force-pushed the feat/inplace-workload-vertical-scaling branch 2 times, most recently from 1f55de3 to bff411c Compare August 28, 2024 06:07
@LavenderQAQ
Copy link
Author

I rebased it first, and I need to take some time to sort through the logic and fill in some tests. After all, this pr is a bit old.

ABNER-1
ABNER-1 requested changes Sep 2, 2024
View reviewed changes
Copy link
Member

@ABNER-1 ABNER-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some methods in VerticalUpdateInterface appear to be unimplemented.

pkg/util/inplaceupdate/inplace_update_defaults.go Outdated Show resolved Hide resolved
pkg/util/inplaceupdate/inplace_update_vertical.go Outdated Show resolved Hide resolved
test/e2e/apps/daemonset.go Outdated Show resolved Hide resolved
@LavenderQAQ @ABNER-1
feat: enhanced in-place update module to support vertical scaling
8392b64 
Signed-off-by: LavenderQAQ <lavenderqaq.cs@gmail.com>
@LavenderQAQ @ABNER-1
test: fixed pre-delete hook tests for advance daemonset
52924e9 
Signed-off-by: LavenderQAQ <lavenderqaq.cs@gmail.com>
@LavenderQAQ @ABNER-1
refactor: define vertically updated interfaces for different implemen…
9fd86b8 
…tations

Signed-off-by: LavenderQAQ <lavenderqaq.cs@gmail.com>
@LavenderQAQ @ABNER-1
feat: remove offstream when vertical update only
9a806d3 
Signed-off-by: LavenderQAQ <lavenderqaq.cs@gmail.com>
@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch 3 times, most recently from 3234854 to 88c2a0a Compare September 11, 2024 13:26
@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch 11 times, most recently from 3f4af6f to a12055b Compare September 14, 2024 09:30
@ABNER-1
Copy link
Member

ABNER-1 commented Sep 14, 2024

I encountered two errors while adding e2e test cases:

  1. When modifying the image and increasing resources simultaneously, I encountered a RunContainerError. I have already submitted an issue to the Kubernetes community at [InPlacePodVerticalScaling]Got RunContainerError when patch pod image and resources kubernetes/kubernetes#127356.

  2. When I scaled the CPU limit of resources from 100m to 200m, I observed that occasionally the status was first changed by kubelet from 100m InProcess to 200m, and then it was set back to 100m InProcess by kubelet, and after a while, it finally became 200m.

@ABNER-1
merge into default handler and add uts/e2e
eea019c 
Signed-off-by: Abner-1 <yuanyuxing.yyx@alibaba-inc.com>
@ABNER-1 ABNER-1 force-pushed the feat/inplace-workload-vertical-scaling branch from a12055b to eea019c Compare September 14, 2024 09:55
LavenderQAQ
LavenderQAQ commented Oct 8, 2024
View reviewed changes
Makefile Show resolved Hide resolved
apis/apps/pub/inplace_update.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ABNER-1 ABNER-1 ABNER-1 requested changes

@FillZpp FillZpp Awaiting requested review from FillZpp

@furykerry furykerry Awaiting requested review from furykerry

@veophi veophi Awaiting requested review from veophi

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
size/XL size/XL: 500-999
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature request] In-place udpate support resources
4 participants
@LavenderQAQ @kruise-bot @codecov-commenter @ABNER-1